Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a --color flag to the CLI and disable colors for Pkl test #571

Closed

Conversation

thomaspurchas
Copy link
Contributor

The --colors flag makes it possible to forcefully disable colour output, and also forcefully enable it. We make use of this in the native Pkl test engines by passing --colors=never into the pkl commands, which ensures we get a predictable output.

Additionally the pkl test command also sets --colors=never to ensure that pkl test output isn't accidentially contaminated with ansi escape chars, which will break example based tests.

The --colors flag makes it possible to forcefully disable colour output,
and also forcefully enable it. We make use of this in the native Pkl
test engines by passing --colors=never into the pkl commands, which
ensures we get a predictable output.

Additionally the pkl test command also sets --colors=never to ensure
that pkl test output isn't accidentially contaminated with ansi escape
chars, which will break example based tests.
Copy link
Contributor

@bioball bioball left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor comments!

Also, this is missing documentation (cli-common-options.adoc, gradle-common-properties.adoc). Do you mind adding them?

@@ -134,6 +134,7 @@ data class CliBaseOptions(

/** Hostnames, IP addresses, or CIDR blocks to not proxy. */
val httpNoProxy: List<String>? = null,
val colors: String = "auto"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's make this an enum:

enum class Color {
  /**
   * Output ANSI-formatted text only when the process STDOUT/STDERR are attached to a TTY.
   */
  AUTO,

  /**
   * Never output ANSI-formatted text.
   */
  NEVER,

  /**
   * Always output ANSI-formatted text.
   */
  ALWAYS
}

And change this type to said enum, and also add a doc comment. Also, nit: s/colors/color

Suggested change
val colors: String = "auto"
/** Whether to output ANSI-formatted text. */
val color: Color = Color.AUTO

Comment on lines +202 to +205
val colors: String by
option(names = arrayOf("--colors"), help = "Enable or disable colour output in the terminal")
.choice("auto", "never", "always")
.default("auto")
Copy link
Contributor

@bioball bioball Jul 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's follow the convention set by unix-y tools like ls, diff, grep, etc, and call this --color.

Also, with the above suggestion to enum-ify this (requires import com.github.ajalt.clikt.parameters.types.enum)

Suggested change
val colors: String by
option(names = arrayOf("--colors"), help = "Enable or disable colour output in the terminal")
.choice("auto", "never", "always")
.default("auto")
val color: Color by
option(names = arrayOf("--color"), help = "Enable or disable colour output in the terminal")
.enum<Color>(key = { it.name.lowercase() })
.default(Color.AUTO)

@@ -230,7 +237,8 @@ class BaseOptions : OptionGroup() {
noProject = projectOptions?.noProject ?: false,
caCertificates = caCertificates,
httpProxy = proxy,
httpNoProxy = noProxy ?: emptyList()
httpNoProxy = noProxy ?: emptyList(),
colors = if (disableColors) "never" else colors,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
colors = if (disableColors) "never" else colors,
color = color

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This exists to provide a mechanism for the test subcommand to forcefully disable not just colours, but also the injection if ANSI codes completely. I've been having issues because the pkl test expected output because they don't travel along the standard output rails. As a consequence the ANSI codes don't get stripped if they're injected.

By providing this escape hatch, I can ensure that colour are disabled before there's any opportunity for the Jansi formatting object to init, because once it's created you can't disable the injection of ANSI codes, only strip them out later. That mixed with the fact that caught errors in pkl test are truncated creates a bit of nightmare as the outputs get truncated to slightly different lengths depending on if colours were enabled, regardless of if those ANSI code actually made it to the output.

I tried to figure out where the error truncation etc was happening, so I could modify it to handle ANSI/non-ANSI outputs in an equivalent way, but I haven't been able to track down where it happens.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think if (when?) we switch to a text formatter, we can control this ourselves by passing a no-op text formatter when --color=never. Then, we're in full control of how formatting happens rather than mutating global state.

Anyways, I don't know if that's relevant to my suggestion here. CliBaseOptions is a data class, and you can always modify it however you want because you get a derived copy method.

val options = baseOptions(myModules).copy(color = Color.NEVER)

Copy link
Contributor Author

@thomaspurchas thomaspurchas Jul 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's only relevant due to how Jansi handles global state. It's caught me out a couple of times because Jansi doesn't handle changes to global state the way you might expect. The final result is what you expect, but the mechanism by which Jansi achieves that result depends on the exact order of operations (mutating global state before or after doing the initial Jansi setup). In most cases it doesn't make any practical difference, but for the specific case of Pkl test it does (which I learned the hard way).

For this specific case I need to make sure the baseOptions are set correctly before the common CLI setup function runs, because that's where Jansi setup happens. It's not enough to only modify the state within the execution context of the test command (which is what I originally tried).

Agree we can just get rid of this hack by implementing a proper formatting API, and having direct control over how control codes are injected. It's also a good argument to do a proper formatting API sooner rather than later. The number of hacks and shortcuts I've had to introduce to get Jansi working as expected is getting pretty uncomfortable at this point. So I'll probably look towards doing that, rather than continuing further with this approach.

@@ -208,7 +214,8 @@ class BaseOptions : OptionGroup() {
fun baseOptions(
modules: List<URI>,
projectOptions: ProjectOptions? = null,
testMode: Boolean = false
testMode: Boolean = false,
disableColors: Boolean = false,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
disableColors: Boolean = false,

@@ -166,7 +166,8 @@ protected CliBaseOptions getCliBaseOptions() {
getTestPort().getOrElse(-1),
Collections.emptyList(),
getHttpProxy().getOrNull(),
getHttpNoProxy().getOrElse(List.of()));
getHttpNoProxy().getOrElse(List.of()),
"auto");
Copy link
Contributor

@bioball bioball Jul 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm pretty sure this means that Gradle will not output any colors, because this is executed as a child process.

Anyway, we should provide the same options in Gradle (auto, never, always). Take a look at how the other options are configured to see what to do here. Or, leave a TODO comment here so we can follow up in a future PR.

@holzensp
Copy link
Contributor

This may be more complicated than what it's worth, but really, pkl test should color the "outside" exceptions and not the "inside" ones, i.e. if pkl test itself throws, it should be colored, but anything inside a module.catch(...) should not.

I don't think this is something that should hold you back, especially considering we want to try and avoid the Jansi dependency in pkl-core. @bioball's suggestion of passing in a formatter does seem the most sensible way to go.

@thomaspurchas
Copy link
Contributor Author

@holzensp yeah my plan to stop working on this approach, and look at passing in a formatter. As I mentioned here, the number of hacks and whatnot that are piling up to make this approach work doesn't feel good. I wouldn't be comfortable merging such a fragile pile of hacks and workarounds.

@bioball
Copy link
Contributor

bioball commented Oct 31, 2024

Closing this; superseded by #746

@bioball bioball closed this Oct 31, 2024
@thomaspurchas thomaspurchas deleted the error-coloring branch November 7, 2024 01:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants